Repository navigation
[vm-repair] Return command warnings in the output payload (2.4.3) - #10340
Conversation
az vm repair create warns when the repair VM size supports NVMe but not SCSI, because repair scripts that locate the attached OS disk by its SCSI model name will not find it there. That warning only ever reached the log, so a script or portal driving the command got a success payload with no way to know the repair VM it just created is one several repair scripts cannot work on. Warnings now also appear under a warnings key, which is omitted when there are none so existing output is unchanged. Raised by the PR 153 validation run in Azure/repair-script-library.
|
Hi Edwin Bernal Microsoft (@EdwinBernal1), |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
🔵 Needs a closer look
Warning propagation and test coverage/teardown issues remain unresolved.
Pull request overview
Updates vm-repair 2.4.3 to expose command warnings in output payloads.
Changes:
- Tracks and conditionally returns warnings.
- Captures disk-controller warnings during VM creation.
- Adds warning tests and updates version history.
File summaries
| File | Summary |
|---|---|
src/vm-repair/setup.py |
Version bump to 2.4.3. |
src/vm-repair/HISTORY.rst |
Documents the warning payload change. |
src/vm-repair/azext_vm_repair/tests/latest/test_returned_warnings.py |
Tests warning behavior; teardown handling and defensive-copy coverage require correction. |
src/vm-repair/azext_vm_repair/custom.py |
Records disk-controller warnings; propagation, deprecation-warning coverage, and create-path testing require changes. |
src/vm-repair/azext_vm_repair/command_helper_class.py |
Adds warning tracking and conditional payload serialization. |
Review details
Suppressed comments (5)
src/vm-repair/azext_vm_repair/custom.py:340
- Because
create()is also called byrepair_and_restore()andrepair_button(), this appends the warning only to the nested helper's payload. Those callers currently logcreate_outand then build a new outer payload without copyingcreate_out['warnings'], so the same NVMe-only condition is still invisible to users of those commands. Propagatecreate_out.get('warnings', [])into the outer command before itsinit_return_dict(), or explicitly scope this feature to directvm repair create.
command.warnings.append(message)
src/vm-repair/azext_vm_repair/custom.py:340
- The added tests exercise
init_return_dictwith a manually populated list, but none execute thiscreatebranch. A regression that drops or misroutes this append would still pass whileaz vm repair createreturned no warning; add a mocked create-path test that forces the selector to return a warning and asserts the final payload contains it.
if level == 'warning':
command.warnings.append(message)
src/vm-repair/azext_vm_repair/custom.py:340
- The payload is still incomplete for the contract described in the PR:
createalso emits the--yesdeprecation warning atcustom.py:198, but only the disk-controller warning is appended here. A caller usingwarningstherefore misses a warning emitted by the same command; either record every warning covered by the contract or narrow the stated guarantee.
if level == 'warning':
command.warnings.append(message)
src/vm-repair/azext_vm_repair/tests/latest/test_returned_warnings.py:20
- These patches are active only while constructing
helper; after the context exits, the returned object is finalized with the realcommand_helper.__del__, even though__init__was skipped. The destructor reads missing attributes such asis_verboseandcmd, producing an ignoredAttributeError/unraisable teardown warning for each test. Use a test subclass with a no-op destructor or otherwise avoid returning a partially initialized production object.
with mock.patch.object(command_helper, '__init__', return_value=None), \
mock.patch.object(command_helper, '__del__', lambda self: None):
src/vm-repair/azext_vm_repair/tests/latest/test_returned_warnings.py:41
- This test mutates
helper.warningsafter creating the return value, so it only checks that the payload is a snapshot. It never verifies the defensive-copy direction promised by the test name: a caller could append toreturnedand corrupthelper.warningsif the original list were returned. Mutatereturnedinstead and assert thathelper.warningsremains unchanged.
returned = helper.init_return_dict()['warnings']
helper.warnings.append('second')
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
[Release] Update index.json for extension [ vm-repair-2.4.3 ] : https://dev.azure.com/msazure/One/_build/results?buildId=181366000&view=results |
🤖 PR Validation — ️✔️ All clear
Summary
vm-repair2.4.3. Advisory warnings raised by a command are now returned in its output under awarningskey, not only written to the log.The problem
az vm repair createwarns when the repair VM size supports NVMe but not SCSI:That warning is real and consequential — several repair scripts in
Azure/repair-script-librarylocate the attached OS disk by its SCSI model name, and on an NVMe-only repair VM they find nothing, complete, and report success. But the warning only ever reached the log. The returned JSON was a plain success payload:{ "status": "SUCCESS", "repair_vm_name": "repair-scr5", ... }So a script, harness, or portal experience driving
az vm repair createhad no way to know that the repair VM it just created is one that several repair scripts cannot work on. The only way to find out was to scrape stderr.This surfaced during the PR #153 validation run in
Azure/repair-script-library, where the warning fired on aStandard_D2ads_v6repair VM and was visible to a human but not to the harness.Changes
azext_vm_repair/command_helper_class.py— the command helper carries awarningslist, andinit_return_dictincludes it. The key is omitted when there are no warnings, so existing output is byte-for-byte unchanged for every command that raises none.azext_vm_repair/custom.py—createrecords the disk-controller warning when the selection produces one. The log line is unchanged; this adds to the payload rather than moving anything.azext_vm_repair/tests/latest/test_returned_warnings.py— new.setup.py,HISTORY.rst— version and changelog.Version & changelog
setup.pyVERSION: 2.4.2 -> 2.4.3HISTORY.rstentry added: yesTesting
test_returned_warnings.pycovers the three things that matter: the key is absent when nothing was raised, a raised warning comes back, and the returned list is a copy so a caller mutating it cannot corrupt the helper's state.Run with the Azure CLI's bundled Python under
unittest.azdev styleandazdev linterwere not run locally —azdevis not on the PATH in this environment, so CI will be the first to exercise them.No live Azure tests were run.
Cross-repo impact (repair-script-library)
None. No run id, script path, or
map.jsonchange.Security review
No new input handling. The only value added to the payload is a fixed message the extension itself produced; no caller-supplied string, resource name, or tag reaches it.
Backward compatibility
Additive. No command, parameter, default, or existing output key changes. A consumer that does not know about
warningssees exactly what it saw before, because the key only appears when a warning was raised.